Skip to content

[SPL] Prevent fclose on underlying SplFileObject file stream. - #7920

Merged
Girgias merged 1 commit into
php:masterfrom
camporter:bug_81691
Jun 20, 2022
Merged

[SPL] Prevent fclose on underlying SplFileObject file stream.#7920
Girgias merged 1 commit into
php:masterfrom
camporter:bug_81691

Conversation

@camporter

Copy link
Copy Markdown
Contributor

To address https://bugs.php.net/bug.php?id=81691

I'm not sure if this is the correct way to address this issue, hence the draft PR.

@cmb69

cmb69 commented Jan 9, 2022

Copy link
Copy Markdown
Member

Well, this wouldn't fix the general issue that the stream must not manipulated without going through the object. But since this can't be prevented as long as the stream is a resource, this looks like a good stop-gap measure for the worst case. Thank you for the PR!

@camporter

camporter commented Jan 10, 2022

Copy link
Copy Markdown
Contributor Author

In the process of testing this I got curious about something with get_resources(). It seems like there's a case where it might be leaking memory? I didn't take the time to dive into it very much...

Can open a separate issue for it though.

Increasing memory usage:

<?php
  ini_set('memory_limit', '8M');
  $i = 0;
  while(true) {
    $file = new SplFileObject(__FILE__);
    $res = get_resources();

    // not setting $res to null
    $file = null;
  
    if ($i == 0) {
      print("mem: " . memory_get_usage() . "\n");
    }
    $i++;
    if ($i == 10000) {
      $i = 0;
    }
  }

Constant memory usage:

<?php
  ini_set('memory_limit', '8M');
  $i = 0;
  while(true) {
    $file = new SplFileObject(__FILE__);
    $res = get_resources();

    $res = null; // set to null
    $file = null;
  
    if ($i == 0) {
      print("mem: " . memory_get_usage() . "\n");
    }
    $i++;
    if ($i == 10000) {
      $i = 0;
    }
  }

@camporter
camporter marked this pull request as ready for review January 10, 2022 14:33
@cmb69

cmb69 commented Jan 10, 2022

Copy link
Copy Markdown
Member

In the process of testing this I got curious about something with get_resources(). It seems like there's a case where it might be leaking memory?

That is not a memory leak in php-src, though. It is basically the same as:

<?php

$all_objs = new WeakMap();
function get_all_objs() {
    global $all_objs;

    $res = [];
    foreach ($all_objs as $obj => $dummy) {
        $res[] = $obj;
    }
    return $res;
}

ini_set('memory_limit', '8M');
$i = 0;
while(true) {
  $obj = new stdClass();
  $all_objs[$obj] = $i;
  $objs = get_all_objs();

//   $objs = null;
  $obj = null;

  if ($i%1000 == 0) {
    print("mem: " . memory_get_usage() . "\n");
  }
  $i++;
}

Calling get_resources() and get_all_objs(), respectively, increases the refcount, and only afterwards the assignment is done where the refcount is decreased. This keeps all resource and objects, respectively, alive forever. If you unset before calling the refcount increasing functions, the refcount drops to zero, and everything is fine.

Anyhow, get_resources() has been introduced with d37820f, and the commit message states:

It may be used for debugging and testing to identify resource leaks.

So it seems important to document that in the PHP manual; users should not use that for any production code (not because of the "memory leak", but as it allows to get to resources which should be in control of an object).

@camporter

Copy link
Copy Markdown
Contributor Author

Ah! Sorry, that makes sense. Thank you 🙂

I opened doc issue php/doc-en#1318 with your suggestions.

@Girgias

Girgias commented Jun 20, 2022

Copy link
Copy Markdown
Member

@cmb69 should this be backported to 8.0 or only master?

@cmb69 cmb69 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think targeting "master" only makes sense, since get_resources() shouldn't be used in production, and we only catch one issue anyway (you could still do other stream manipulations, which likely break SplFileObjects internals, but some code might actually do that deliberately).

@Girgias
Girgias merged commit 6bd0175 into php:master Jun 20, 2022
adrian-enspired pushed a commit to adrian-enspired/php-src that referenced this pull request Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants